Codecov Report

Merging #71 (a32d54a) into master (6efbec2) will increase coverage by 0.15%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##           master      #71      +/-   ##
==========================================
+ Coverage   92.60%   92.75%   +0.15%     
==========================================
  Files           8        8              
  Lines         473      483      +10     
  Branches       86       91       +5     
==========================================
+ Hits          438      448      +10     
  Misses         23       23              
  Partials       12       12              

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6efbec2...a32d54a. Read the comment docs.

Hello @simonthor. I hope you're doing well.

Shall we try and give this a push?

That would be a good change indeed, I won't be able to assist unfortunately as I am on vacation the next twoish weeks, after that I can support fully

Yes, I will likely have time to work on this now. Have I understood it correctly that the goal is to add another parameter to the GenMultiDecay.from_dict called e.g. particle_model_map that will be a dict of the form {"D0":"gauss", ...}?

More or less. See the original discussion at #63 (comment).

The part to enhance is https://github.com/zfit/phasespace/blob/master/phasespace/fromdecay/genmultidecay.py#L62-L86. Your example is

            >>> parser = DecFileParser('example_decays.dec')
            >>> parser.parse()
            >>> dst_chain = parser.build_decay_chains("D*+")
            >>> dst_chain
            {'D*+': [{'bf': 0.984,
                'fs': [{'D0': [{'bf': 1.0,
                        'fs': ['K-', 'pi+']}]},
                    'pi+']},
                {'bf': 0.016,
                 'fs': ['D+', 'gamma']}]}
            If the D0 particle should have a mass distribution of a gaussian when it decays into K- and pi+
            a `zfit` parameter can be added to its decay dict:
            >>> dst_chain["D*+"][0]["fs"][0]["D0"][0]["zfit"] = "gauss"
            >>> dst_chain
            {'D*+': [{'bf': 0.984,
                'fs': [{'D0': [{'bf': 1.0,
                        'fs': ['K-', 'pi+'],
                        'zfit': 'gauss'}]},
                    'pi+']},
                {'bf': 0.016,
                'fs': ['D+', 'gamma']}]}
           >>> dst_gen = GenMultiDecay.from_dict(dst_chain)

but, clearly, we do not want users to have to do things such as dst_chain["D*+"][0]["fs"][0]["D0"][0]["zfit"] = "gauss".
The problem can be solved with an API of the kind

from decaylanguage import DecayMode, DecayChain
dm = DecayMode(1, 'K- pi+', model='PHSP', zfit="gauss")
dc = DecayChain('D0', {'D0':dm})
dst_chain = dc.to_dict()
dst_gen = GenMultiDecay.from_dict(dst_chain)

specifying that you want to model the D0 decay with a Gauss function.

In addition, you can also allow for extra flexibility in the GenMultiDecay.from_dict constructor with a new argument to pass for example {"D0":"gauss", ...} as you mention.

To me it makes sense to have these 2 ways. That's powerful.

I definitely agree that the current way of adding mass functions is clunky! I have added a particle_model_map argument now but some tests seem to fail, so I will try to fix that.

The problem can be solved with an API of the kind

from decaylanguage import DecayMode, DecayChain
dm = DecayMode(1, 'K- pi+', model='PHSP', zfit="gauss")
dc = DecayChain('D0', {'D0':dm})
dst_chain = dc.to_dict()
dst_gen = GenMultiDecay.from_dict(dst_chain)

specifying that you want to model the D0 decay with a Gauss function.

I think this API should already work. since dc.to_dict() outputs a regular DecayLanguage dict.
Maybe this example should be added as an example in the documentation? I did not add it before, since I thought the main point of GenMultiDecay is to handle when there are multiple possible decays, while a DecayChain only supports one way for a particle to decay.

The feature has now been added and works as intended, according to tests. I will try to increase the code coverage and add some more documentation and then I think it is ready to be merged! Unless you think there is anything missing?

Allow me a couple more days to get back to you.

Of course! Sorry for the slow progress on my side as well. Other projects with deadlines pop up, which I then have to prioritize.

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

Hello @simonthor! Been a while ... the CI failures seem very easy to sort out. Can you try and push this to the end?

Hello! Sorry, I missed that the tests were failing but I should have fixed it now.
Pre-commit CI seems to fail on a file that I have not touched and the Codacy errors seem to be the same issue I had with the last pull request, so I feel ready to remove the WIP status!

Go ahead and remove the WIP tag ...

Morning @simonthor, can you rebase given my Flake8 fixes in #74? That should clean-up the pre-commit failures you are seeing above. Then I hope we can approve and merge.

I tried rebasing now and the pre-commit errors seem to be gone. Thank you for the fix!

Cool. As for the Codacy "errors" I see on the linked page above, https://app.codacy.com/gh/zfit/phasespace/pullRequest?prid=8615307, that it's trivial to get those gone. Nicer if you do that.

@jonas-eschle, going to make a release as soon as this is in :-)?

Ah, you're getting the same errors as on master and I see @jonas-eschle already created an issue on that, see DanielBok/nlopt-python#12.

Ah, you're getting the same errors as on master and I see @jonas-eschle already created an issue on that, see DanielBok/nlopt-python#12.

I see, got a bit confused when the rebase somehow caused all tests to fail.

Cool. As for the Codacy "errors" I see on the linked page above, https://app.codacy.com/gh/zfit/phasespace/pullRequest?prid=8615307, that it's trivial to get those gone. Nicer if you do that.

This is an "error" I encountered last pull request as well. The docformatter pre-commit hook moves the summary to the first line, while Codacy prefers the summary to be on the second line. So I think either Codacy or docformatter will have to be reconfigured.

Ah, for Scikit-HEP we use Black as formatter, and we have not been using Codacy.

I guess those errors can be ignored since unrelated to this MR. That would be my pragmatic approach here.

Yep, the errors of codacy can be removed, we actually switched already to black since a long time, I'll deactivate this

zfit deps should be fixed now, as soon as it's available again we can rerun the CI and the merge it! Fine from your side @simonthor ?

zfit deps should be fixed now, as soon as it's available again we can rerun the CI and the merge it! Fine from your side @simonthor ?

That is fine by me! All tests seem to pass now, besides black in pre-commit which apparently has an ImportError. However, since I run pre-commit locally on all code I push here, there should not be any problems. So I think this branch is ready to be merged :)

Well done 👍!

That is fine by me! All tests seem to pass now, besides black in pre-commit which apparently has an ImportError. However, since I run pre-commit locally on all code I push here, there should not be any problems. So I think this branch is ready to be merged :)

I agree, LGTM! Since you've been contributing a lot in the past and your coding is on a pretty good level, I've added you as a developer (still figuring out the right rights in Github), I think you should now be able to write to this repository yourself. Bear in mind, that with great power comes great responsibility and it's never wrong to ask first and oc always do changes as we have done now.
I guess you also understand the codebase good enough to be able to judge commits or PRs well.

So feel free to merge this PR yourself and welcome to the project!

Thank you both for the help and Jonas for inviting me to zfit!